Skip to content

Conversation

wtrocki
Copy link
Contributor

@wtrocki wtrocki commented Dec 19, 2016

Motivation

Allow to stream logs for ongoing builds.
Current methods doesn't allow that and users need to wait for entire build to get full build log.

This PR is implementation for #217

@khmarbaise
Copy link
Member

Could please offer unit/integration tests....?

@wtrocki
Copy link
Contributor Author

wtrocki commented Dec 19, 2016

@khmarbaise - yes, as said above - I wanted to do it if this idea would be accepted, but there is not much involved to do it anyway. Going to do it now.

@khmarbaise
Copy link
Member

I can say that. I already accept your idea and improvements and in particular your contributions...which is really great....

@wtrocki wtrocki force-pushed the jenkins-client-217 branch 2 times, most recently from 41a72b0 to d52b5c1 Compare December 19, 2016 18:31
@wtrocki
Copy link
Contributor Author

wtrocki commented Dec 19, 2016

Thanks for approval. Final cuts added. Current code base wasn't affected by my changes, only new methods were introduced. Unit tests were added. I reverted pooling method to be blocking. We can squash this into single commit when merging.

/**
* Listener interface used to obtain build console logs
*/
public interface BuildConsoleStreamListener {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about timeout ?

Copy link
Contributor Author

@wtrocki wtrocki Dec 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With async version (previous commit) all errors including timeouts were handled by error method here. Since we use now blocking method - (I believe it's more cleaner way to do it that way) , all errors would be handled as exceptions.

@khmarbaise
Copy link
Member

Can you please take a look onto the travis build cause it's failed....

@matzew
Copy link

matzew commented Dec 20, 2016

@matzew
Copy link

matzew commented Dec 20, 2016

@khmarbaise in case this is merged, can you again publish a new snapsht :)

Thanks 🍻

@wtrocki
Copy link
Contributor Author

wtrocki commented Dec 29, 2016

Any chance to get that merged?

@khmarbaise
Copy link
Member

Is it possible to squash all those commits together and make a issue reference in the log as I mentioned in the contribution part of the README...

@wtrocki wtrocki force-pushed the jenkins-client-217 branch from b091881 to 66f8f12 Compare January 2, 2017 19:40
@wtrocki
Copy link
Contributor Author

wtrocki commented Jan 2, 2017

Really sorry. I was confused because in ReleaseNotes reference github issues. I created and referenced github issue before creating this PR: #217

Going to move this to jira now.
Jira issue was created: https://issues.jenkins-ci.org/browse/JENKINS-40753
Next time I would do it properly.

@wtrocki wtrocki force-pushed the jenkins-client-217 branch 2 times, most recently from 887532c to 41986d5 Compare January 3, 2017 08:50
@wtrocki
Copy link
Contributor Author

wtrocki commented Jan 3, 2017

Failing build item is unrelated with may changes. I tried to run this on my local machine and I was passing! It looks like timing issue.

@wtrocki wtrocki force-pushed the jenkins-client-217 branch 2 times, most recently from 59beea5 to 3a8bf5a Compare January 5, 2017 17:16
@wtrocki
Copy link
Contributor Author

wtrocki commented Jan 5, 2017

Tried to resolve problem. It looks like master is broken, but only on travis.

@matzew
Copy link

matzew commented Jan 5, 2017 via email

@wtrocki wtrocki closed this Jan 20, 2017
@wtrocki wtrocki reopened this Jan 20, 2017
@wtrocki wtrocki force-pushed the jenkins-client-217 branch from 3a8bf5a to ae589d5 Compare January 30, 2017 12:54
@khmarbaise
Copy link
Member

khmarbaise commented Feb 3, 2017

Yes the master had an issue sorry for that. Now the master is fixed (my fault ;-(().
And your change looks good to me...once another rebase against master please...

@wtrocki wtrocki force-pushed the jenkins-client-217 branch from ae589d5 to 4c59d43 Compare February 4, 2017 14:48
@wtrocki
Copy link
Contributor Author

wtrocki commented Jun 10, 2017

Build is passing. Can we get that merged?

@khmarbaise khmarbaise added this to the Release 0.3.8 milestone Jun 10, 2017
@khmarbaise khmarbaise merged commit ff91fd6 into jenkinsci:master Jun 10, 2017
@wtrocki wtrocki deleted the jenkins-client-217 branch June 10, 2017 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants